Feat/rust reqwest shallow object query param #21321
Conversation
linxGnu
left a comment
linxGnu
left a comment
There was a problem hiding this comment.
@whirm thank you for re-opening the PR.
Let's check isModel property with @wing328 for the insight.
If isModel can not guarantee flat-object, I am with the idea from @wing328, either:
- Generating Rust code but warn user about about malform specification of Query Params. Reference: https://swagger.io/docs/specification/v3_0/serialization/
- Find another indicator instead of using
isModel
Beside above concern, the PR is LGTM.
| {{#isModel}} | ||
| let params = crate::apis::parse_flat_object(&serde_json::to_value({{{vendorExtensions.x-rust-param-identifier}}})?); | ||
| req_builder = req_builder.query(¶ms); | ||
| {{/isModel}} |
There was a problem hiding this comment.
Please correct me If I am wrong @wing328: isModel here does not guarantee non-nested?
There was a problem hiding this comment.
isModel can have nested properties (properties that are models)
There was a problem hiding this comment.
Good catch.
There's an issue though, it seems like the isDeepObject flag is not correctly set for nested objects. I've added a couple test cases to the PR.
@wing328 am I doing something wrong here?
There was a problem hiding this comment.
I think isDeepObject also does not guarantee non-nested.
Generated code from @whirm integration test:
pub fn list(configuration: &configuration::Configuration, page_query: models::ListPageQueryParameter, page_query_schema: models::Page, deep: models::ListDeepParameter, not_required: Option<models::ListNotRequiredParameter>, not_required_deep: Option<models::ListNotRequiredDeepParameter>)
...
if let Some(ref param_value) = p_not_required_deep {
let params = crate::apis::parse_flat_object(&serde_json::to_value(param_value)?);
req_builder = req_builder.query(¶ms);
}
...However, regarding specs, it's nested object:
- in: query
name: notRequiredDeep
required: false
schema:
type: object
properties:
foo:
type: object
properties:
baz:
type: integer
format: int32
bar:
type: integer
format: int32There was a problem hiding this comment.
I'm back from vacation and with some time to look at this again. @wing328 is isDeepObject bugged or I'm not understanding its semantics and should be using something else instead?
whirm
force-pushed
the
feat/rust-reqwest-shallow-object-query-param
branch
from
bf72fae to
8c9b3c1
Compare
…y params for testing
whirm
force-pushed
the
feat/rust-reqwest-shallow-object-query-param
branch
from
8c9b3c1 to
16e500e
Compare
3 participants
Recreating #21199 as it turns out one can't reopen a PR after force-pushing to its branch.
objectQueryParam.yamlto cover both required and optional variants.Note that in some cases it changes the existing behavior (serialize object to JSON string) to flattening the object. Which AFAIU is what the 3.0 specs ("schema vs content" subsection) indicate as the correct behavior. I've double checked by running such a spec with swagger UI and inspecting the generated queries.
PR checklist
Commit all changed files.
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*.IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
master(upcoming7.x.0minor release - breaking changes with fallbacks),8.0.x(breaking changes without fallbacks)@frol, @farcaller, @richardwhiuk, @paladinzh, @jacob-pro, @linxGnu Ready to review!